Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose memory.use_hierarchy in MemoryStats #1378

Merged
merged 1 commit into from
Jun 30, 2017

Conversation

derekwaynecarr
Copy link
Contributor

Fixes #1376

Signed-off-by: Derek Carr decarr@redhat.com

Copy link
Contributor

@vishh vishh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rh-atomic-bot
Copy link

245/252 passed on RHEL - Failed.
243/250 passed on CentOS - Failed.
249/252 passed on Fedora - Failed.
Log - https://aos-ci.s3.amazonaws.com/opencontainers/runc/runc-integration-tests-prs/298/fullresults.xml

@mrunalp
Copy link
Contributor

mrunalp commented Mar 20, 2017

@vbatts TRAVIS_COMMIT_RANGE isn't getting set for this PR failing git-validation. What was the workaround for that?

@mrunalp
Copy link
Contributor

mrunalp commented Mar 20, 2017

We could do something like this https://github.com/mendersoftware/mendertesting/pull/9/files

@vbatts
Copy link
Member

vbatts commented Mar 21, 2017

@mrunalp let me check on that right now

vbatts added a commit to vbatts/git-validation that referenced this pull request Mar 21, 2017
opencontainers/runc#1378 (comment)
points to an alternate approach.

Reported-by: Mrunal Patel <mrunalp@gmail.com>
Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
@mrunalp
Copy link
Contributor

mrunalp commented Mar 21, 2017

#1382 should fix the issue with travis.

wking added a commit to wking/git-validation that referenced this pull request Mar 21, 2017
Master builds only have a 'git clone ...' [1] so FETCH_HEAD isn't
defined and git-validation crashes [2].  This commit partially reverts
8a12a8f (main: default travis commit range is unreliable, 2017-03-21,
vbatts#13) to avoid that crash.  If TRAVIS_COMMIT_RANGE is unset [3],
falling back to TRAVIS_COMMIT may be fine.

The ... -> .. replacement works around travis-ci/travis-ci#4596 until
that is fixed upstream [4].  This avoids pulling in commits from the
base tip that aren't reachable from the head tip (e.g. if master has
advanced since the PR branched off, and the PR is against master).  We
only want to check commits that are in the head branch but not in the
base branch (more details on the range syntax in [5]).

Once the Travis bug does get fixed, the shell replacement will be a
no-op.  So we don't have to worry about checks breaking once the bug
gets fixed, and can periodically poll the bug and remove the
workaround at out leisure after the fix.

[1]: https://travis-ci.org/opencontainers/runc/jobs/213508696#L243
[2]: https://travis-ci.org/opencontainers/runc/jobs/213508696#L347
[3]: opencontainers/runc#1378 (comment)
[4]: travis-ci/travis-ci#4596
[5]: http://git-scm.com/docs/gitrevisions#_specifying_ranges

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/git-validation that referenced this pull request Mar 21, 2017
Master builds only have a 'git clone ...' [1] so FETCH_HEAD isn't
defined and git-validation crashes [2].  This commit partially reverts
8a12a8f (main: default travis commit range is unreliable, 2017-03-21,
vbatts#13) to avoid that crash.  If TRAVIS_COMMIT_RANGE is unset [3],
falling back to TRAVIS_COMMIT may be fine.

The ... -> .. replacement works around travis-ci/travis-ci#4596 until
that is fixed upstream [4].  This avoids pulling in commits from the
base tip that aren't reachable from the head tip (e.g. if master has
advanced since the PR branched off, and the PR is against master).  We
only want to check commits that are in the head branch but not in the
base branch (more details on the range syntax in [5]).

Once the Travis bug does get fixed, the shell replacement will be a
no-op.  So we don't have to worry about checks breaking once the bug
gets fixed, and can periodically poll the bug and remove the
workaround at out leisure after the fix.

[1]: https://travis-ci.org/opencontainers/runc/jobs/213508696#L243
[2]: https://travis-ci.org/opencontainers/runc/jobs/213508696#L347
[3]: opencontainers/runc#1378 (comment)
[4]: travis-ci/travis-ci#4596
[5]: http://git-scm.com/docs/gitrevisions#_specifying_ranges

Signed-off-by: W. Trevor King <wking@tremily.us>
@mrunalp
Copy link
Contributor

mrunalp commented Mar 22, 2017

LGTM

Approved with PullApprove

@@ -243,6 +243,14 @@ func (s *MemoryGroup) GetStats(path string, stats *cgroups.Stats) error {
}
stats.MemoryStats.KernelTCPUsage = kernelTCPUsage

use_hierarchy := strings.Join([]string{"memory", "use_hierarchy"}, ".")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should be UseHierarchy

Signed-off-by: Derek Carr <decarr@redhat.com>
@derekwaynecarr
Copy link
Contributor Author

@runcom @mrunalp - fixed nit.

@rh-atomic-bot
Copy link

267/275 passed on RHEL - Failed.
259/273 passed on CentOS - Failed.
273/274 passed on Fedora - Failed.
Log - https://aos-ci.s3.amazonaws.com/opencontainers/runc/runc-integration-tests-prs/311/fullresults.xml

@mrunalp
Copy link
Contributor

mrunalp commented Mar 31, 2017

LGTM

Approved with PullApprove

@runcom
Copy link
Member

runcom commented Mar 31, 2017

👍

@derekwaynecarr
Copy link
Contributor Author

@mrunalp @vishh -- anything else needed on this? i wanted to get back around to fixing cAdvisor

@dqminh
Copy link
Contributor

dqminh commented Jun 30, 2017

LGTM

Approved with PullApprove

@dqminh dqminh merged commit 7139b61 into opencontainers:master Jun 30, 2017
openshift-merge-robot added a commit to openshift/origin that referenced this pull request Sep 19, 2017
Automatic merge from submit-queue

cadvisor/runc updates

Support CRI-O:
google/cadvisor#1741

Fix memory stats:
google/cadvisor#1728
opencontainers/runc#1378

@derekwaynecarr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants